Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: also gracefully shutdown on SIGTERM #17802

Merged
merged 2 commits into from
Jul 30, 2024
Merged

feat: also gracefully shutdown on SIGTERM #17802

merged 2 commits into from
Jul 30, 2024

Conversation

BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Jul 24, 2024 •
edited
Loading

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

...because Kubernetes will send SIGTERM when killing a pod.

As described in #17662, this can empower more seamless scaling-in in Kubernetes deployments. (correct me if I'm wrong)

After this PR, I suppose we can get rid of the extra manual step of risingwave ctl meta unregister-worker when scaling-in, as described in the doc.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

The step 1 of manual unregistration of the compute node when scaling-in (doc) is not necessary any more. Users can directly apply the new yaml file to trigger a graceful scale-in.

@BugenZhao BugenZhao requested review from shanicky and arkbriar July 24, 2024 10:09
BugenZhao
BugenZhao commented Jul 24, 2024
View reviewed changes
src/utils/runtime/src/lib.rs Outdated
@@ -96,24 +97,26 @@ where
spawn_prof_thread(profile_path);
}

let mut sigint = tokio::signal::unix::signal(SignalKind::interrupt()).unwrap();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't support Windows, so directly use unix-only features here.

@BugenZhao BugenZhao force-pushed the bz/shutdown-sigterm branch from 74a5dbe to fc9d5cd Compare July 25, 2024 06:48
@graphite-app graphite-app bot requested a review from a team July 25, 2024 06:48
@BugenZhao BugenZhao changed the base branch from main to bz/more-graceful-shutdown July 25, 2024 06:48
@BugenZhao BugenZhao mentioned this pull request Jul 25, 2024
Merged
4 tasks
This was referenced Jul 25, 2024
Merged
Merged
@BugenZhao Graphite App
Copy link
Member Author

BugenZhao commented Jul 25, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @BugenZhao and the rest of your teammates on Graphite Graphite

@BugenZhao BugenZhao mentioned this pull request Jul 25, 2024
Merged
4 tasks
@BugenZhao BugenZhao force-pushed the bz/shutdown-sigterm branch from fc9d5cd to c36de8a Compare July 25, 2024 06:49
@BugenZhao BugenZhao force-pushed the bz/more-graceful-shutdown branch from 8c20bad to 4dcfb9d Compare July 26, 2024 08:26
@BugenZhao BugenZhao force-pushed the bz/shutdown-sigterm branch from d0515b1 to 6188e69 Compare July 26, 2024 08:26
Base automatically changed from bz/more-graceful-shutdown to main July 26, 2024 10:16
@graphite-app graphite-app bot requested a review from a team July 26, 2024 10:16
BugenZhao added 2 commits July 29, 2024 18:01
@BugenZhao
feat: also gracefully shutdown on SIGTERM
e408bcc 
Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao
fix it
766b0b0 
Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao BugenZhao force-pushed the bz/shutdown-sigterm branch from 6188e69 to 766b0b0 Compare July 29, 2024 10:01
stdrc
stdrc approved these changes Jul 30, 2024
View reviewed changes
src/utils/runtime/src/lib.rs
// Watch SIGINT, typically originating from user pressing ctrl-c.
// Attempt to shutdown gracefully and force shutdown on the next signal.
_ = sigint.recv() => {
tracing::info!("received ctrl-c, shutting down... (press ctrl-c again to force shutdown)");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@BugenZhao
Copy link
Member Author

Tested with risingwave-operator and it works like a charm. 🥰

compute:

2024-07-30T09:10:05.276715082Z  INFO risingwave_rt: received SIGTERM, shutting down...
2024-07-30T09:10:05.283413439Z  INFO risingwave_rpc_client::meta_client: successfully unregistered from meta service worker_id=2004
2024-07-30T09:10:05.283480937Z  WARN risingwave_stream::task::barrier_manager: shutdown with running actors, scaling or migration will be triggered
2024-07-30T09:10:05.283581726Z  INFO risingwave_stream::task::barrier_manager: waiting for meta service to close control stream...
2024-07-30T09:10:05.285297803Z  INFO risingwave_rpc_client::meta_client: Heartbeat loop is stopped
2024-07-30T09:10:05.285344052Z  INFO risingwave_common::telemetry::report: Telemetry exit

meta:

2024-07-30T09:10:05.283791678Z  WARN risingwave_meta::barrier::info: node with running actors is deleted node_id=2004 node=Some(WorkerNode { id: 2004, r#type: ComputeNode, host: Some(HostAddress { host: "risingwave-compute-2.risingwave-compute", port: 5688 }), state: Running, property: Some(Property { is_streaming: true, is_serving: true, is_unschedulable: false }), transactional_id: Some(4), resource: None, started_at: None, parallelism: 1 }) actors={2007, 2008}
2024-07-30T09:10:05.283967173Z  INFO failure_recovery{error=worker node 2004 is shutting down prev_epoch=6887532391759872}: risingwave_meta::barrier::recovery: recovery start!
2024-07-30T09:10:05.287488493Z  INFO risingwave_meta::hummock::manager::worker: Released hummock context 2004
2024-07-30T09:10:05.288955702Z  INFO failure_recovery{error=worker node 2004 is shutting down prev_epoch=6887532391759872}:recovery_attempt: risingwave_meta::manager::catalog::fragment: cleaning dirty downstream merge nodes for table sink
2024-07-30T09:10:05.293052214Z  INFO failure_recovery{error=worker node 2004 is shutting down prev_epoch=6887532391759872}:recovery_attempt: risingwave_meta::barrier::recovery: recovering mview progress
2024-07-30T09:10:05.293080297Z  INFO failure_recovery{error=worker node 2004 is shutting down prev_epoch=6887532391759872}:recovery_attempt: risingwave_meta::barrier::recovery: recovered mview progress
2024-07-30T09:10:05.304094784Z  INFO failure_recovery{error=worker node 2004 is shutting down prev_epoch=6887532391759872}:recovery_attempt: risingwave_meta::barrier::recovery: offline rescheduling for job 2001 in recovery is done

@BugenZhao BugenZhao removed the request for review from a team July 30, 2024 09:17
@BugenZhao BugenZhao added this pull request to the merge queue Jul 30, 2024
@BugenZhao BugenZhao added the user-facing-changes Contains changes that are visible to users label Jul 30, 2024
Merged via the queue into main with commit b8e08c7 Jul 30, 2024
35 of 37 checks passed
@BugenZhao BugenZhao deleted the bz/shutdown-sigterm branch July 30, 2024 09:44
@fuyufjh
Copy link
Member

fuyufjh commented Jul 31, 2024

cc. @arkbriar

@BugenZhao BugenZhao mentioned this pull request Sep 18, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stdrc stdrc stdrc approved these changes

@shanicky shanicky Awaiting requested review from shanicky

@arkbriar arkbriar Awaiting requested review from arkbriar

Assignees
No one assigned
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BugenZhao @fuyufjh @stdrc